Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use .To4() on serverId #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use .To4() on serverId #14

wants to merge 1 commit into from

Conversation

tfheen
Copy link
Contributor

@tfheen tfheen commented Apr 15, 2015

The server id should be four octets, per RFC1533, so call .To4 on it
to ensure we have a v4 address and not a mapped v4 address.

@tfheen
Use .To4() on serverId
8697bd5 
The server id should be four octets, per RFC1533, so call .To4 on it
to ensure we have a v4 address and not a mapped v4 address.
@tfheen
Copy link
Contributor Author

tfheen commented Apr 15, 2015

I wasn't entirely sure if you wanted to keep the API, .To4() can return nil if serverId is a v6 address, in which case the ReplyPacket should probably fail. Not using .To4() means you can end up sending a 16 octet long ServerIdentifier, which doesn't work with all DHCP clients (at least the ISC DHCP client is unhappy with it).

@krolaw
Copy link
Owner

krolaw commented Apr 16, 2015

I'm going to think about this some more.
If IPv4 nochange.
If mapped v4 corrects a potential bug.
If IPv6 goes from invalid serverId (too long) to invalid serverId (zero length).

However behaviour would be inconsistent between ReplyPacket and .AddOption which could make a bug harder to find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tfheen @krolaw